Skip to content

[Mobile Payments] Specific alert provider for built-in reader connections #8299

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Dec 5, 2022
Merged

[Mobile Payments] Specific alert provider for built-in reader connections #8299

merged 2 commits into from
Dec 5, 2022

Conversation

@joshheald
Copy link
Contributor

@joshheald joshheald commented Dec 2, 2022 •
edited
Loading

Part of: #8082

Description

In sTAP Away, we're adding support for Apple's built in card readers to take in-person payments. We continue to use Stripe's SDK to handle payments.

Stripe's connection flow is technically very similar to the bluetooth reader flow, but conceptually, there are differences and it's important that we make it clear to users that they are using the built in reader, and don't give the wrong impression that they are connecting to a bluetooth reader, to avoid confusion.

This PR adds an alert provider specialised for connection to a built-in reader, along with new alert view models and temporary text and images for the scanning and connecting stages of the connecting flow.

It does not handle error states, or "software updates" – these will be done in a future PR.

There are also new protocols, splitting up the alerts so that when we refactor the two connection controllers to share duplicated code, they can work with the alert providers as opaque types.

Testing instructions

Using an iPhone (XS or above on iOS 15.4 or newer, with a passcode set and signed in to iCloud)

  1. Navigate to Menu > Settings > Experimental features
  2. Turn on Tap to Pay on iPhone
  3. Navigate to Menu > Payments > Collect payment
  4. Go through the payment flow, and select Card on the payment method screen
  5. When asked for a reader type, tap Tap to Pay on iPhone and go through the Terms of Service Apple ID linking (if you've not done so before)
  6. Observe that specific messages for the built in reader are shown during the connecting process (note that images and text are all placeholders!)

Screenshots

specific-ttpoi-alerts.mp4
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@joshheald
8082 Add ConnectionAlertsProviders for each flow
58cd33b 
Built-in and Bluetooth connection flows are similar (technically) but different in the eyes of users. We will need to show different alerts even though we are doing similar work in the connection code.

This adds various AlertsProviding protocols and two concrete providers, for the bluetooth and built in flows. Both implementations show the same alerts in this commit, but they will be specialised in upcoming commits.

When we refactor the connection controllers, the shared code can use a `CardReaderConnectionAlertsProviding` for the common alerts, without knowing which flow it is serving.
@joshheald joshheald added the feature: mobile payments Related to mobile payments / card present payments / Woo Payments. label Dec 2, 2022
@joshheald joshheald added this to the 11.6 milestone Dec 2, 2022
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 2, 2022 •
edited
Loading

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8299-dce1972 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@joshheald joshheald force-pushed the issue/8082-add-specific-alertprovider-for-built-in-reader-connections branch from daeafb5 to cf3dbf0 Compare December 2, 2022 18:23
@joshheald joshheald changed the base branch from trunk to issue/8082-simplify-BuiltInCardReaderConnectionController December 2, 2022 18:33
@joshheald joshheald changed the title Issue/8082 add specific alertprovider for built in reader connections [Mobile Payments] Specific alert provider for built-in reader connections Dec 2, 2022
@joshheald joshheald added the type: enhancement A request for an enhancement. label Dec 2, 2022
@joshheald joshheald force-pushed the issue/8082-add-specific-alertprovider-for-built-in-reader-connections branch from cf3dbf0 to dce1972 Compare December 2, 2022 18:40
@joshheald joshheald requested a review from toupper December 2, 2022 19:37
@joshheald joshheald marked this pull request as ready for review December 2, 2022 19:41
Base automatically changed from issue/8082-simplify-BuiltInCardReaderConnectionController to trunk December 5, 2022 09:44
@toupper toupper self-assigned this Dec 5, 2022
@toupper
Copy link
Contributor

toupper commented Dec 5, 2022

The code looks good and it tests well, thanks for the PR @joshheald!

I observed one behavior when tapping to pay with my iPhone, it is not strictly related to this PR but It's worth mentioning. After paying with the card successfully and the native iOS modal is dismissed, we still show briefly the previous message that advises the user to present the card or retry, which is confusing to the user.
For instance in this case I had to retry the card tap in the native iOS modal. After it was successful with a second try and the modal was dismissed, we still showed "Retry Card" for a while:

If we pay successfully after one try, we show briefly the previous message:

toupper
toupper approved these changes Dec 5, 2022
View reviewed changes
...merce/Classes/ViewModels/CardPresentPayments/CardPresentModalBuiltInConnectingToReader.swift

/// Modal presented when we are connecting to a reader
///
final class CardPresentModalBuiltInConnectingToReader: CardPresentPaymentsModalViewModel {
Copy link
Contributor

@toupper toupper Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A non-blocker suggestion here, I would make these namings easier to understand, perhaps CardPresentBuiltInConnectingToReaderModalViewModel and CardPresentBuiltInReaderCheckingDeviceSupportModalViewModel? I see they are long, so I leave it to your discernment.

Copy link
Contributor Author

@joshheald joshheald Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I'll put a note on the issue to update these when we have finalised strings and images for the screens: there's not really a lot of clarity over what the SDK is actually doing at each point, so naming them based on the message is probably the clearest approach.

...asses/ViewRelated/Dashboard/Settings/CardReadersV2/CardReaderConnectionAlertsProviding.swift
continueSearch: @escaping () -> Void,
cancelSearch: @escaping () -> Void) -> CardPresentPaymentsModalViewModel

// TODO: implement this approach, allowing us to remove the several readers logic from CardPresentPaymentAlertsPresenter
Copy link
Contributor

@toupper toupper Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented code here?

Copy link
Contributor Author

@joshheald joshheald Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's deliberate, I'll be bringing it back in soon.

@joshheald
Copy link
Contributor Author

joshheald commented Dec 5, 2022

Thanks for the review @toupper

I observed one behavior when tapping to pay with my iPhone, it is not strictly related to this PR but It's worth mentioning. After paying with the card successfully and the native iOS modal is dismissed, we still show briefly the previous message that advises the user to present the card or retry, which is confusing to the user. For instance in this case I had to retry the card tap in the native iOS modal. After it was successful with a second try and the modal was dismissed, we still showed "Retry Card" for a while:

If we pay successfully after one try, we show briefly the previous message:

Yes, this is known... it's covered by #8289: I've not got to the payment flows yet, and this is an existing issue that is a lot more obvious when the phone is the reader 😊

@joshheald joshheald merged commit e75248d into trunk Dec 5, 2022
@joshheald joshheald deleted the issue/8082-add-specific-alertprovider-for-built-in-reader-connections branch December 5, 2022 10:29
@joshheald joshheald mentioned this pull request Dec 5, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toupper toupper toupper approved these changes

Assignees

@toupper toupper

Labels

feature: mobile payments Related to mobile payments / card present payments / Woo Payments. type: enhancement A request for an enhancement.

Projects

None yet

Milestone

11.6

Development

Successfully merging this pull request may close these issues.

4 participants

@joshheald @wpmobilebot @toupper